fix: emit the wire request as raw UTF-8 so the challenge id reproduces - #185
Open
EfeDurmaz16 wants to merge 2 commits into
Open
fix: emit the wire request as raw UTF-8 so the challenge id reproduces#185EfeDurmaz16 wants to merge 2 commits into
EfeDurmaz16 wants to merge 2 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
generate_challenge_id() canonicalizes the request with ensure_ascii=False, but _b64_encode(), which format_www_authenticate() uses to build the wire `request` auth-param, relied on json.dumps' ensure_ascii default of True. For any non-ASCII byte the HMAC bound one string while the header carried another, so the challenge id could not be reproduced from the value on the wire. Section 5.1.1 of draft-httpauth-payment-00 binds the id to the base64url-encoded request as it appears on the wire, so the HMAC input and the emitted parameter have to be the same bytes. This SDK still verifies its own challenges because verify() decodes the echoed value back to a dict and re-canonicalizes it, which is why the mismatch was invisible locally and only surfaced against other SDKs. The same rule was duplicated across nine call sites and four of them had drifted. Route them all through a single canonical_json() helper so the forms cannot diverge again.
EfeDurmaz16
force-pushed
the
fix/canonical-json-non-ascii
branch
from
July 28, 2026 14:40
34067f2 to
742c1f4
Compare
Recompute the challenge id the way a peer verifier does, from the `request` value parsed out of the WWW-Authenticate header, and assert it matches the id the server minted. The existing unicode vector only exercises generate_challenge_id(), which was already correct, so nothing covered the wire form.
EfeDurmaz16
force-pushed
the
fix/canonical-json-non-ascii
branch
from
July 28, 2026 14:45
742c1f4 to
754de0b
Compare
Author
|
@brendanjryan could you take a look, or point me at the right reviewer? Short version: for any non-ASCII byte in the request, the challenge id is computed over one serialization and the header carries another, so a peer verifier cannot reproduce the id. pympp still verifies its own challenges because Workflows are sitting at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
generate_challenge_id()serializes the request withensure_ascii=False, but the code that writesthe
requestparameter into the header does not. For any non-ASCII character the HMAC signs onestring while the header carries another, so a peer cannot recompute the id and rejects the challenge
with
invalid-challenge.Section 5.1.1 of
draft-httpauth-payment-00binds the id to the request as it appears on the wire,so the two have to be the same bytes.
Sites that had drifted to the
ensure_ascii=Truedefault:_parsing.py:43(_b64_encode), reached fromformat_www_authenticateat:169extensions/mcp/types.py:92,190,195Before
sequenceDiagram autonumber participant S as pympp server participant P as Peer verifier Note over S: request contains a non-ASCII character S->>S: generate_challenge_id(), ensure_ascii=False Note right of S: id = HMAC(raw UTF-8 bytes) S->>S: format_www_authenticate(), ensure_ascii=True Note right of S: header carries escaped bytes S-->>P: 402 with that id, but escaped request P->>P: recompute HMAC from the header value Note right of P: HMAC(escaped bytes), does not match P-->>S: rejected, invalid-challengeAfter
sequenceDiagram autonumber participant S as pympp server participant P as Peer verifier Note over S: request contains a non-ASCII character S->>S: generate_challenge_id() via canonical_json() S->>S: format_www_authenticate() via canonical_json() Note right of S: one definition, same bytes S-->>P: 402 with that id, raw UTF-8 request P->>P: recompute HMAC from the header value Note right of P: same bytes, matches P-->>S: acceptedWhy the tests missed it
verify()decodes the echoed request back to a dict and re-serializes it before recomputing the id,so pympp always agrees with itself. The mismatch only appears against another implementation.
test_unicode_in_descriptioncoversgenerate_challenge_id(), which was already correct. Nothingcovered the header.
The fix
The same serialization rule appeared at nine call sites and four of them had drifted. This routes
all nine through one
canonical_json()helper.Read
_parsing.pyfirst, it holds the fix. Then__init__.py,extensions/mcp/types.pyand_body_digest.py, which just call the helper, then the tests.Commit 1 is the behavior change, commit 2 is the tests. Both pass on their own.
Testing
780 passed, 41 skipped(778 onmain, plus the 2 new tests).To watch the tests fail without the fix, cherry-pick the test commit onto
mainand runuv run pytest tests/test_challenge_id.py -k WireForm.One thing worth a second opinion
The smallest possible fix is four
ensure_ascii=Falsearguments. I used a shared helper instead,which touches
__init__.pyand_body_digest.pyeven though they were already correct. My reasoningis that nine copies of one rule, four of them drifted, is the actual problem. If you would rather
keep the diff minimal, I will cut it back.
Only non-ASCII requests change. ASCII output is byte-identical, so existing challenge ids are
unaffected, and the ids that do change are ones no other implementation could verify anyway.